Skip to content

Fix issue 274 #281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Fix issue 274 #281

wants to merge 6 commits into from

Conversation

T-Tmnr
Copy link

@T-Tmnr T-Tmnr commented Nov 15, 2018

Fix issues on Windows platform using Docker.

  • Appended windowsVerbatimArguments option to spawnSync on docker.js
  • Fix bindPath to enclose by double-quart on docker.js.
  • When dockerizePip is enabled and Win32 platform, switch pythonBin from python.exe to python.

@dschep
Copy link
Contributor

dschep commented Nov 15, 2018

@adanot and @gbrcmg does this fix work for you too? Install with:

npm i -g T-Tmnr/serverless-python-requirements#fix-issue-274

@heri16 @kichik, does this look good?

@kichik
Copy link
Contributor

kichik commented Nov 15, 2018

I'm not sure how that would work based on this comment:

#274 (comment)

With quotes on bindPath itself it will produce commands with:

-v something:"something with space"

But the comment suggests (and it seems logically correct) this is required:

-v "something:something with space"

@dschep
Copy link
Contributor

dschep commented Nov 15, 2018

@kichik, does the inclusion of windowsVerbatimArguments not affect this choice?

@kichik
Copy link
Contributor

kichik commented Nov 15, 2018

I think it just tells it not to convert -v test:"hello world" to -v "test:"hello world"".

@T-Tmnr
Copy link
Author

T-Tmnr commented Nov 16, 2018

I modified code using modification on v4.2.5 fully.

@kichik -v "something:something with space" is also correct.
I try to fix the issue by this pattern on my another branch.
https://github.com/T-Tmnr/serverless-python-requirements/tree/fix-issue-274-another
But it's revert changes at dockerPathForWin of v4.2.5.

windowsVerbatimArguments option tells 'not quoating cmdOptions on windows platform'.
In this case cmdOptions=['-v', 'test:"hello world"'] without windowsVerbatimArguments,
quating 'test:"hello world"' to '"test:"hello world""'.
Finally, the spawned command got arguments bellow.

$1=-v 
$2="test:\"hello 
$3=world\"" 

@T-Tmnr
Copy link
Author

T-Tmnr commented Nov 16, 2018

I posted to the issue.
#274 (comment)

@kichik
Copy link
Contributor

kichik commented Nov 17, 2018

My two cents on this:

  1. -v "something:something with spaces" is more correct than -v something:"something with space" and doesn't rely on any special treatment of quotes.
  2. It will be easier in the future if we separate quote handling from command generation. We might have more code paths that we're missing here or that will be added. I personally prefer letting the code doing the actual execution deal with the quoting. In this case it would be spawnSync() without windowsVerbatimArguments. If it doesn't know how to properly quote, let's create a wrapper for it that does. But handling the quotes in the code that builds the command will mean that has to be done in any code that calls a command.

@bsamuel-ui
Copy link
Contributor

bsamuel-ui commented Nov 27, 2018

It will be easier in the future if we separate quote handling from command generation.

I'm doing exactly that in #292 for exactly the reason you mentioned.

@dschep dschep closed this in #288 Nov 29, 2018
dschep added a commit that referenced this pull request Nov 29, 2018
#288)

…-quate -v option.

Remove enclosing by double-quate at dockerPathForWin on Windows platform.
It's caused bad effect on spawnSync without shell=true or windowsVerbatimArguments=true.

closes #274 
closes #281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants